fix(rust): make 100 * pl.col(pl.Boolean).mean() work #13725
Conversation
Wainberg
requested review from
ritchie46,
stinodego,
c-peters,
alexander-beedie,
MarcoGorelli and
orlp
as code owners
| @@ -127,7 +127,11 @@ impl AExpr { | |||
| Mean(expr) => { | |||
| let mut field = | |||
| arena.get(*expr).to_field(schema, Context::Default, arena)?; | |||
| float_type(&mut field); | |||
| if matches!(&field.dtype, DataType::Boolean) { | |||
There was a problem hiding this comment.
This branch already happens in the float_type call?
Edit. I see that there are two float_type functions that are inconsistent. Boolean should be added to the float_type function, not as an exclusion here.
There was a problem hiding this comment.
I'll be honest, I just blindly copied @advoet's code from the linked issue. But it does fix the bug.
There was a problem hiding this comment.
It does fix it, but it is a bandaid.
There was a problem hiding this comment.
Happy to implement a more robust fix, just let me know where the "right" place to change would be. It's an important fix because people like to use 100 * boolean_column.mean() to calculate percentages.
There was a problem hiding this comment.
There are a lot of cases where boolean is treated as a u8 in these types of operations. It might be worth adding is_numeric_or_bool() for those many cases scattered throughout the codebase (perhaps different PR).
There was a problem hiding this comment.
Eh, I just perused through and it feels like it's not worth it. Go ahead with this one.
There was a problem hiding this comment.
I'm just not familiar enough with the Rust codebase to know what a non-bandaid fix would look like.
There was a problem hiding this comment.
He just means at the very top of the file do this instead of your else clause below:
fn float_type(field: &mut Field) {
if (field.dtype.is_numeric() || field.dtype == DataType::Boolean)
&& field.dtype != DataType::Float32
{
field.coerce(DataType::Float64)
}
}There was a problem hiding this comment.
ohhh I didn't see Ritchie's edit to his original post! thanks :)
There was a problem hiding this comment.
He may have been noting the other float_type function in expr_dyn_fn.rs but this is actually never used.
Wainberg
changed the title
|
@ritchie46 updated this PR based on your suggestion:
|
Co-authored-by: Wainberg <m.wainberg@utoronto.ca>
Closes #7651 based on the implementation suggested by @advoet here.
Before:
After: